Skip to content

Conversation

@derpyspike
Copy link
Contributor

Hi!

This PR changes the UI check in RNodeConfigValidator and its test on RNodeConfigValidatorTest to allow using Spreading Factors 5 and 6 too instead of just 7-12.
Both the Reticulum RNSD as well as RNode work perfectly with these SFs and we've already tried them both with Meshtastic and RNodes and they work without issues with many different hardwares, so IMHO they shouldn't be disabled.
Also we're looking into using SF6 in our Reticulum LoRa Mesh and that would allow Columba to be used with RNodes on this mesh too.

Also thanks for your great work on Columba!🙂

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Jan 28, 2026

Greptile Overview

Greptile Summary

Updated the spreading factor validation to allow SF5 and SF6 in addition to SF7-12.

Changes:

  • Modified MIN_SF constant from 7 to 5 in RNodeConfigValidator.kt:52
  • Updated test boundary cases to validate SF5 as the new minimum and SF4 as below-minimum
  • Error messages now correctly display "Must be >= 5" instead of "Must be >= 7"

Analysis:
The change is consistent with existing code documentation (RNodeRegionalPreset.kt:78 already documents "LoRa SF (5-12)") and aligns with Reticulum RNSD and RNode hardware capabilities. All validation logic remains intact - only the minimum bound changed. Tests comprehensively cover the new boundaries.

Confidence Score: 5/5

  • This PR is safe to merge with no concerns
  • Simple constant change with comprehensive test coverage; aligns with existing documentation and hardware capabilities
  • No files require special attention

Important Files Changed

Filename Overview
app/src/main/java/com/lxmf/messenger/viewmodel/RNodeConfigValidator.kt Changed MIN_SF from 7 to 5, allowing SF5 and SF6; change is minimal and correct
app/src/test/java/com/lxmf/messenger/viewmodel/RNodeConfigValidatorTest.kt Updated boundary tests to validate SF5 and SF4 edge cases correctly

Sequence Diagram

sequenceDiagram
    participant User
    participant UI
    participant RNodeConfigValidator
    participant ValidationResult

    User->>UI: Enter SF value (e.g., "5", "6")
    UI->>RNodeConfigValidator: validateSpreadingFactor(value)
    
    alt value is blank
        RNodeConfigValidator->>ValidationResult: FieldValidation(isValid=true)
        Note over RNodeConfigValidator: Allow empty while typing
    else value is not a number
        RNodeConfigValidator->>ValidationResult: FieldValidation(isValid=false, "Invalid number")
    else value < MIN_SF (5)
        RNodeConfigValidator->>ValidationResult: FieldValidation(isValid=false, "Must be >= 5")
    else value > MAX_SF (12)
        RNodeConfigValidator->>ValidationResult: FieldValidation(isValid=false, "Must be <= 12")
    else valid range (5-12)
        RNodeConfigValidator->>ValidationResult: FieldValidation(isValid=true)
    end
    
    ValidationResult->>UI: Return validation result
    UI->>User: Display validation feedback
Loading

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 files reviewed, 2 comments

Edit Code Review Agent Settings | Greptile

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Jan 28, 2026

Additional Comments (1)

app/src/test/java/com/lxmf/messenger/viewmodel/RNodeConfigValidatorTest.kt
test new minimum boundary SF5 instead of SF7

With MIN_SF changed to 5, this should test SF5 as the minimum boundary, not SF7.

        assertTrue(RNodeConfigValidator.validateSpreadingFactor("5").isValid)
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/src/test/java/com/lxmf/messenger/viewmodel/RNodeConfigValidatorTest.kt
Line: 194:194

Comment:
test new minimum boundary SF5 instead of SF7

With `MIN_SF` changed to 5, this should test SF5 as the minimum boundary, not SF7.

```suggestion
        assertTrue(RNodeConfigValidator.validateSpreadingFactor("5").isValid)
```

How can I resolve this? If you propose a fix, please make it concise.

derpyspike and others added 2 commits January 28, 2026 21:19
…datorTest.kt with suggestion from greptile-apps bot


Fix the error message in the RNodeConfigValidatorTest to SF5 instead of SF7 (Suggestion from greptile-apps)

Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
@derpyspike
Copy link
Contributor Author

Additional Comments (1)

app/src/test/java/com/lxmf/messenger/viewmodel/RNodeConfigValidatorTest.kt test new minimum boundary SF5 instead of SF7

With MIN_SF changed to 5, this should test SF5 as the minimum boundary, not SF7.

        assertTrue(RNodeConfigValidator.validateSpreadingFactor("5").isValid)

Prompt To Fix With AI

Ok I made the two suggested fixes from the greptile-apps bot.

Should I squash the three commits into one or is it fine like this?

@serialrf433
Copy link
Contributor

And where in the code is the filter to not try to do SF5 on SX127x chipset based RNodes?
Take a look at page 27 of the datasheet: https://www.semtech.com/products/wireless-rf/lora-connect/sx1276

@derpyspike
Copy link
Contributor Author

And where in the code is the filter to not try to do SF5 on SX127x chipset based RNodes? Take a look at page 27 of the datasheet: https://www.semtech.com/products/wireless-rf/lora-connect/sx1276

If I'm not mistaken I think that logic is on the RNode firmware, at least that's how it rnsd seems to do it.
For example on a Heltec V4 you could set 28dBm as TX power and the RNode 1.85 accepts that value on the V4 and calculates the txpower but if you do that on other RNodes, the RNode doesn´t validate the settings and doesn´t work.
I don´t know if that's something Columba can check beforehand🤷🏻‍♀️

@serialrf433
Copy link
Contributor

Sort of.
https://github.com/markqvist/RNode_Firmware/blob/master/sx127x.cpp#L382

Its just setting SF6 if you set SF5 on the SX127x

The same logic is in the SX126x where its setting SF5 if you set for example SF4
https://github.com/markqvist/RNode_Firmware/blob/master/sx126x.cpp#L699

This could confuse people who just try to set things and do not know why its working on the one and not on the other device. The target for Columba are still the users without deep technical knowledge.

But in general i would say, it is a benefit to have the ability to do something. Maybe at some later change there should be something to explain the users why things are working as they do.

Here you can take a look at the same SF7 limit discussion in other Reticulum projects: markqvist/Reticulum#1036

@derpyspike
Copy link
Contributor Author

Yeah, I mean in our mesh all of the nodes are SX1262 or similar, and for the same reason we are running a custom Meshtastic firmware for the Meshtastic mesh.
The thing is with the limitation Sideband and Columba are left out of the question, where you can use rnsd without issue.

The possibility should be there, and someone inputing custom values should probably know if their hardware supports that, just like for example you could input 433MHz on a 868MHz RNode and that wouldn´t be a valid config. But how do you check for that?

@serialrf433
Copy link
Contributor

serialrf433 commented Jan 28, 2026

And do not forget the SX127x handle SF6 different then newer chips.

Quote:
The SX127x supports SF6 only in implicit mode (fixed packet length, no header).

Source:
Datasheet of newer chips that say the SF6 of the SX127x is not compatible with the SF6 of more modern chips when you implement it in a way that differs to the SX127x implementation. Page 146 of the datasheet of the LR2021: https://www.semtech.com/products/wireless-rf/lora-plus/lr2021

@torlando-tech
Copy link
Owner

Additional Comments (1)
app/src/test/java/com/lxmf/messenger/viewmodel/RNodeConfigValidatorTest.kt test new minimum boundary SF5 instead of SF7
With MIN_SF changed to 5, this should test SF5 as the minimum boundary, not SF7.

        assertTrue(RNodeConfigValidator.validateSpreadingFactor("5").isValid)

Prompt To Fix With AI

Ok I made the two suggested fixes from the greptile-apps bot.

Should I squash the three commits into one or is it fine like this?

no need to squash. thank you! always take the greptile comments with a grain of salt :)

@sentry
Copy link

sentry bot commented Jan 28, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@torlando-tech
Copy link
Owner

@greptileai

@torlando-tech torlando-tech merged commit a51e8e4 into torlando-tech:main Jan 28, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants